fix: support buffers greater than `2^32` bytes in length in `Buffer.concat` and `Buffer.copy` #55492

duncpro · 2024-10-22T15:49:23Z

This pull request...

Adds support for Buffer.copy with lengths greater than or equal to 2^32 bytes.
Adds support for Buffer.concat where the result buffer has length greater than or equal to 2^32 bytes.
Improves the docs for Buffer.copy.
- They did not mention the behavior when the source slice cannot fit into the target slice.
- They did not mention the behavior when sourceEnd exceeds the total number of bytes in the source buffer.
Refactors implementation of Buffer.copy and Buffer.concat.
- Adds asserts verifying its impossible to copy bytes from outside the source buffer.
- Adds asserts verifying it's impossible to copy bytes anywhere except the target buffer.

avivkeller · 2024-10-22T16:06:22Z

Hi! Can you please amend your commit message to follow the guidelines of this repository? It needs to be under 80 characters

avivkeller · 2024-10-22T16:07:00Z

Additionally, can you add a test to validate this behavior

duncpro · 2024-10-22T17:32:57Z

I noticed that parallel/test-stream-readable-unpipe-resume is failing on Ubuntu CI, which is strange because it's not failing on my Mac. I'll investigate.

avivkeller · 2024-10-22T17:33:44Z

I noticed that parallel/test-stream-readable-unpipe-resume is failing on Ubuntu CI, which is strange because it's not failing on my Mac. I'll investigate.

No need, it's a flaky test. It's a known process deadlock that's being investigated.

test/parallel/test-buffer-concat.js

avivkeller · 2024-10-22T17:54:33Z

@ronag I've requested your review because you were involved in the issue this closes, and you know a lot about buffers that I don't

test/parallel/test-buffer-concat.js

duncpro · 2024-10-22T20:42:17Z

Don't merge this yet. It's possible to read bytes from outside the source buffer into the target buffer.
The current implementation _copyActual does not check for negatives in source and target.

For example...

function _copyActual(source, target, targetStart, sourceStart, sourceEnd) {
  if (sourceEnd - sourceStart > target.byteLength - targetStart)
    sourceEnd = sourceStart + target.byteLength - targetStart;

  let nb = sourceEnd - sourceStart;
  const sourceLen = source.byteLength - sourceStart;
  if (nb > sourceLen)
    nb = sourceLen;

  if (nb <= 0)
    return 0;

  // _copy(source, target, targetStart, sourceStart, nb);

  // return nb;

   return [source, target, targetStart, sourceStart, nb];
}

let src = Buffer.alloc(50);
let dest = Buffer.alloc(100);
const [_, __, ___, sourceStart, copy_len] = _copyActual(src, dest, 0, -10, 0); 
console.log([sourceStart, copy_len]); // [ -10, 10 ]

Its probably better here not to trust the caller of _copyActual and to guard the call to _copy with explicit checks.
Our choice is to either throw an error, or to silently clamp the bounds. I prefer the former.

Resolved by: 3c18a13

avivkeller · 2024-10-22T21:26:08Z

@nodejs/buffer PTAL ☝️

duncpro · 2024-10-22T22:01:45Z

I've updated _copyActual to guard _copy (native binding here) against negative numbers, so there is no risk of potentially allowing out of bounds access in the future.

The expected behavior for an out of bounds copy is actually specified by test/parallel/test-buffer-copy.js.

At the latest commit 3c18a13, test/parallel/test-buffer-copy.js is passing (along with everything else).

codecov · 2024-10-22T23:54:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (7bc3e16) to head (5ca703f).
Report is 1807 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #55492    +/-   ##
========================================
  Coverage   88.41%   88.41%            
========================================
  Files         653      654     +1     
  Lines      187435   187575   +140     
  Branches    36077    36081     +4     
========================================
+ Hits       165714   165851   +137     
+ Misses      14960    14954     -6     
- Partials     6761     6770     +9

Files with missing lines	Coverage Δ
lib/buffer.js	`100.00% <100.00%> (ø)`
src/node_buffer.cc	`70.36% <100.00%> (+0.10%)`	⬆️

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

duncpro · 2024-10-23T00:50:45Z

Not sure what do about code coverage issue. It seems to me that the range check should stay in as preventative measure.

node/lib/buffer.js

Line 247 in 0bc0b81

throw new ERR_BUFFER_OUT_OF_BOUNDS('expected 0 <= sourceStart <= sourceEnd' +
node/lib/buffer.js

Line 255 in 0bc0b81

throw new ERR_BUFFER_OUT_OF_BOUNDS('expected 0 <= targetStart <= target.byteLength' +

avivkeller · 2024-10-23T00:57:44Z

No need to worry about codecov, it's not 100% accurate

ronag · 2024-10-23T07:33:46Z

lib/buffer.js

    return 0;

+  // Guaranteed 0 <= sourceStart < sourceEnd (NO GUARANTEE <= source.byteLnegth)
+  // Guaranteed 0 <= targetStart < target.byteLength here


I think we could just make these asserts. The perf impact is negligable...

ronag · 2024-10-23T07:33:51Z

lib/buffer.js


+// Assumes `source`, `target` are real buffers.
+// Assumes 0 <= sourceStart <= sourceEnd
+// Assumes 0 <= targetStart <= target.byteLength


I think we could just make these asserts. The perf impact is negligable...

node/lib/buffer.js

Lines 271 to 275 in 209289c

assert(isUint8Array(source) && isUint8Array(target));

// Enforce: 0 <= sourceStart <= sourceEnd <= source.byteLength

assert(0 <= sourceStart && sourceStart <= sourceEnd && sourceEnd <= source.byteLength);

// Enforce: 0 <= targetStart<= target.byteLength

assert(0 <= targetStart && targetStart <= target.byteLength);

rotemdan · 2024-10-23T08:52:22Z

Turns out the issue also occurs in Buffer.copy. See my comment on the issue thread.

avivkeller · 2024-10-24T15:59:12Z

lib/buffer.js

    if (targetStart < 0)
      throw new ERR_OUT_OF_RANGE('targetStart', '>= 0', targetStart);
  }
+  // Guaranteed 0 <= targetStart


What does this mean?

Shouldn't an assert be used instead?

If not, IMO a comment isn't needed at all

I agree it is redundant. This comment has been removed as of commit 5ca703f.

avivkeller · 2024-10-24T16:03:18Z

test/parallel/test-buffer-concat.js

+// Test its possible to concat buffers where the total length of the result in bytes
+// is greater than 2^32.


Suggested change

// Test its possible to concat buffers where the total length of the result in bytes

// is greater than 2^32.

// Ref: ...

Comment is replaced with a reference to the issue as of commit 5ca703f.

avivkeller · 2024-10-24T16:03:24Z

test/parallel/test-buffer-copy.js

 }
+
+
+// Test its possible to copy buffer with length > 2^32


Comment is replaced with a reference to the issue as of commit 5ca703f.

duncpro · 2024-10-30T07:34:23Z

@redyetidev This is no longer WIP unless you have anymore concerns or suggestions.

nodejs-github-bot · 2024-10-30T09:02:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63349/

duncpro · 2024-11-05T23:38:55Z

I believe CI is failing because the runner is exhausting its memory.

We do some pretty large allocations in the unit tests.

node/test/parallel/test-buffer-copy.js

Lines 239 to 245 in 5ca703f

    
           // Ref: https://github.com/nodejs/node/issues/55422#issue-2594486812 
        
           if (2 ** 32 + 1 <= kMaxLength) { 
        
             const src = Buffer.alloc(2 ** 32 + 1, 1); 
        
             const dst = Buffer.alloc(2 ** 32 + 1, 2); 
        
             src.copy(dst); 
        
             assert.deepStrictEqual(src, dst); 
        
           }

node/test/parallel/test-buffer-concat.js

Lines 103 to 113 in 5ca703f

    
           // Ref: https://github.com/nodejs/node/issues/55422#issue-2594486812 
        
           if (2 ** 32 + 1 <= kMaxLength) { 
        
             const a = Buffer.alloc(2 ** 31, 0); 
        
             const b = Buffer.alloc(2 ** 31, 1); 
        
             const c = Buffer.alloc(1, 2); 
        
             const destin = Buffer.concat([a, b, c]); 
        
             assert.deepStrictEqual(destin.subarray(0, 2 ** 31), a); 
        
             assert.deepStrictEqual(destin.subarray(2 ** 31, 2 ** 32), b); 
        
             assert.deepStrictEqual(destin.subarray(2 ** 32), c); 
        
           }

Perhaps the tests should be skipped?

jasnell · 2024-11-05T23:39:23Z

lib/buffer.js

+// - The source slice is not clamped to fit into the target slice. If it won't fit, this throws.
+// - If either the source or target slice are out of bounds, this throws.
+function _copyActual(source, target, targetStart, sourceStart, sourceEnd) {
+  assert(isUint8Array(source) && isUint8Array(target));


The assert here is a bit redundant given the checks that are made in the copyImpl call site.

In my opinion this assert is useful since it makes _copyActual's invariant that source and target need to be Uint8Array explicit.

It indicates to any future/new maintainers needing to call _copyActual what the invariants are. They don't have to guess at the invariants, or deduce the preconditions by exploring all the call-sites of _copyActual.

Ah. This is breaking

This works intentionally since #54088:

> Buffer.of(1, 2, 3, 4).copy(x = new Uint16Array(4)), x Uint16Array(4) [ 513, 1027, 0, 0 ]

And it can't be reverted without a semver-major

#54088 just missed to add tests, #60399 will add them

jasnell · 2024-11-05T23:40:51Z

test/parallel/test-buffer-concat.js

                                      new Uint8Array([0x43, 0x44])]),
                       Buffer.from('ABCD'));
+
+// Ref: https://github.com/nodejs/node/issues/55422#issue-2594486812


Nit: please consider expanding the comment a bit here with a short summary of what is being tested for so folks don't have to necessarily follow the link to get the gist.

I agree that it is better to have the description of the test inlined.

However I was under the impression that the policy was to use refs?

See #55492 (comment) where I actually removed the explanation at the request of another maintainer.

ChALkeR · 2025-10-31T20:33:27Z

What's the status of this?
It looks like this PR got blocked because of the new tests are not functioning on CI?
Does it really need those new tests to get landed or could they be a separate PR?

ChALkeR · 2025-10-31T20:35:29Z

lib/buffer.js

+  // Enforce: 0 <= sourceStart <= sourceEnd <= source.byteLength
+  assert(0 <= sourceStart && sourceStart <= sourceEnd && sourceEnd <= source.byteLength);
+  // Enforce: 0 <= targetStart<= target.byteLength
+  assert(0 <= targetStart && targetStart <= target.byteLength);


This should also likely fix #59985

ChALkeR · 2025-10-31T20:36:02Z

lib/buffer.js

+  const targetCapacity = target.byteLength - targetStart;
+  assert(copyLength <= targetCapacity);
+
+  _copy(source, target, targetStart, sourceStart, copyLength);


_copy is slower than set but #60399 would fix that

ChALkeR · 2025-10-31T20:40:28Z

lib/buffer.js

+// - The source slice is not clamped to fit into the target slice. If it won't fit, this throws.
+// - If either the source or target slice are out of bounds, this throws.
+function _copyActual(source, target, targetStart, sourceStart, sourceEnd) {
+  assert(isUint8Array(source) && isUint8Array(target));


Ah. This is breaking

This works intentionally since #54088:

> Buffer.of(1, 2, 3, 4).copy(x = new Uint16Array(4)), x Uint16Array(4) [ 513, 1027, 0, 0 ]

And it can't be reverted without a semver-major

#54088 just missed to add tests, #60399 will add them

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Oct 22, 2024

duncpro force-pushed the slow-copy-u64 branch from 03d27e2 to d15c192 Compare October 22, 2024 16:01

duncpro force-pushed the slow-copy-u64 branch from d15c192 to cd0a59a Compare October 22, 2024 16:08

avivkeller added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Oct 22, 2024

avivkeller reviewed Oct 22, 2024

View reviewed changes

test/parallel/test-buffer-concat.js Outdated Show resolved Hide resolved

avivkeller requested a review from ronag October 22, 2024 17:54

avivkeller reviewed Oct 22, 2024

View reviewed changes

test/parallel/test-buffer-concat.js Outdated Show resolved Hide resolved

ronag approved these changes Oct 22, 2024

View reviewed changes

avivkeller added the wip Issues and PRs that are still a work in progress. label Oct 22, 2024

ronag reviewed Oct 23, 2024

View reviewed changes

duncpro changed the title ~~fix: support concatenating buffers larger than 4GB on 64-bit systems~~ fix: support buffers greater than 2^32 bytes in length in Buffer.concat and Buffer.copy Oct 24, 2024

duncpro changed the title ~~fix: support buffers greater than 2^32 bytes in length in Buffer.concat and Buffer.copy~~ fix: support buffers greater than $2^32$ bytes in length in Buffer.concat and Buffer.copy Oct 24, 2024

duncpro changed the title ~~fix: support buffers greater than $2^32$ bytes in length in Buffer.concat and Buffer.copy~~ fix: support buffers greater than 2^32 bytes in length in Buffer.concat and Buffer.copy Oct 24, 2024

avivkeller reviewed Oct 24, 2024

View reviewed changes

duncpro force-pushed the slow-copy-u64 branch from 21b9ce0 to 5ca703f Compare October 25, 2024 03:38

buffer: support copy/concat for >= 2^32 bytes

5ca703f

BridgeAR approved these changes Oct 28, 2024

View reviewed changes

BridgeAR added request-ci Add this label to start a Jenkins CI on a PR. and removed wip Issues and PRs that are still a work in progress. labels Oct 30, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 30, 2024

jasnell reviewed Nov 5, 2024

View reviewed changes

ChALkeR reviewed Oct 31, 2025

View reviewed changes

ChALkeR suggested changes Oct 31, 2025

View reviewed changes

	assert(isUint8Array(source) && isUint8Array(target));
	// Enforce: 0 <= sourceStart <= sourceEnd <= source.byteLength
	assert(0 <= sourceStart && sourceStart <= sourceEnd && sourceEnd <= source.byteLength);
	// Enforce: 0 <= targetStart<= target.byteLength
	assert(0 <= targetStart && targetStart <= target.byteLength);

		// Test its possible to concat buffers where the total length of the result in bytes
		// is greater than 2^32.

	// Test its possible to concat buffers where the total length of the result in bytes
	// is greater than 2^32.
	// Ref: ...

Uh oh!

fix: support buffers greater than 2^32 bytes in length in Buffer.concat and Buffer.copy #55492

Are you sure you want to change the base?

fix: support buffers greater than 2^32 bytes in length in Buffer.concat and Buffer.copy #55492

Uh oh!

Conversation

duncpro commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avivkeller commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avivkeller commented Oct 22, 2024

Uh oh!

duncpro commented Oct 22, 2024

Uh oh!

avivkeller commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

avivkeller commented Oct 22, 2024

Uh oh!

Uh oh!

duncpro commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avivkeller commented Oct 22, 2024

Uh oh!

duncpro commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

duncpro commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avivkeller commented Oct 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rotemdan commented Oct 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duncpro commented Oct 30, 2024

Uh oh!

nodejs-github-bot commented Oct 30, 2024

Uh oh!

duncpro commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duncpro Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChALkeR Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fix: support buffers greater than `2^32` bytes in length in `Buffer.concat` and `Buffer.copy` #55492

fix: support buffers greater than `2^32` bytes in length in `Buffer.concat` and `Buffer.copy` #55492

duncpro commented Oct 22, 2024 •

edited

Loading

avivkeller commented Oct 22, 2024 •

edited

Loading

avivkeller commented Oct 22, 2024 •

edited

Loading

duncpro commented Oct 22, 2024 •

edited

Loading

duncpro commented Oct 22, 2024 •

edited

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading

duncpro commented Oct 23, 2024 •

edited

Loading

duncpro commented Nov 5, 2024 •

edited

Loading

duncpro Nov 6, 2024 •

edited

Loading

ChALkeR Oct 31, 2025 •

edited

Loading

ChALkeR Oct 31, 2025 •

edited

Loading